[Platform] Use capabilities instead of instanceof checks #299
Conversation
…ring pre-request situations to reduce dependency on specific model class
…ace during pre-request situations to reduce dependency on specific model class
…e during response situations to reduce dependency on specific model class
JoshuaBehrens
requested review from
chr-hertel,
Nyholm and
OskarStark
as code owners
carsonbot
changed the title
|
Hey @JoshuaBehrens, I think you are on a good topic here, but we def need to align before we roll out changes like that - also looking at #300. I think as well that we'll need some changes on how the Platform works, and I was working on #136 to bring in a new mechanism for result conversion and reducing the repetitive implementations. And I def need to wrap that up before we gonna move on Capabilities/Actions/Tasks/etc. |
|
@chr-hertel fine to me :) I admit I expected it to start a discussion instead of an LGTM merge. I also assume, that we gonna need some model metadata to check before invoking it. Some instanceof check sort-of promised existence of the model which now is a lot of trust towards the user. And in ollama we already have some checks about embeddings and somewhere else is a listModels function. So there is also need for that :) Depending on the way "we need to align" I am not sure whether Github issue commentary is a helpful chat alternative |
carsonbot
changed the title
| public function supports(Model $model): bool | ||
| { | ||
| return $model instanceof Gemini; | ||
| return $model->supports(Capability::INPUT_MESSAGES); |
There was a problem hiding this comment.
The way those model are used in the request method is
$url = \sprintf(
'https://generativelanguage.googleapis.com/v1beta/models/%s:%s',
$model->getName(),
$options['stream'] ?? false ? 'streamGenerateContent' : 'generateContent',
);
Which means that the name should be a valid model name for Gemini.
While it's kinda implicit when using a Gemini Model (and should be enforced by phpdoc), it's not the cade for a generic model...
So I dunno if you should have something like
return $model->supports(Capability::INPUT_MESSAGES)
&& \in_array($model->getName(), Gemini::MODELS, true)
Same for others supports methods.
There was a problem hiding this comment.
This is intended with my comment on
This eventually will reduce the amount of code you have to write to establish new providers that replicate existing API's with a well-known subset of models or add better support for custom models like OpenAI fine-tuned models or ollamas self-built models.
I am with you, that it would be great if you know in before whether the model exists but the API can tell with a 404. No need to update the library to support new models.
|
@JoshuaBehrens Thanks for pointing me to this PR. I tried to come up with an alternative approach that introduces a new PHP attribute like For instance, this would allow to create a custom What do you think? |
|
What about Ofc we need to make sure, that you can only register one ModelClient per platform and capability. |
I like the idea, probably some more attributes might he helpful (I'd like to play around with that in some demo code to get a better feeling). What would In general I'm seeking for a way to resolve the (current) strict coupling of components by using attributes. An in addition, having a way to discover the "feature set" (models, clients, capabilities) more easily with dependency injection (I see, that DI is out of scope for this lib, but it would be helpful for project integrations - introducing attributes/tags are just perfect for that). |
|
Basically yes, but this file should be updated by syncing or so, like we do for the ICU data for example Symfony |
|
@ohader I don't get the benefit of the attribute yet. It feels like you need a different way to configure model classes so one can mark existing model classes for new platforms. So how would one extend it? |
This commit bricks some code flows. Other changes needs to be done as well. See #300
In other issues and pull requests the model class should be used as a simple struct, only holding scalar values and should not have any business logic effect by inheritance. This has been the case for embeddings, completions and other API calls like Whisper audio, and Dall-e images. Right now this makes it difficult to share different instances of the model class with various providers, although a lot of providers share the same models, and therefore likely the same capabilities. This eventually will reduce the amount of code you have to write to establish new providers that replicate existing API's with a well-known subset of models or add better support for custom models like OpenAI fine-tuned models or ollamas self-built models.
To stop this, I have changed all instanceof checks matching a specific model subclass to refer to the expected input or output capabilities of the model, that were implemented in the specific messagenormalizer, resultconverter or modelclient. It was sometimes not easy to decide, which approach to go as not every input has a corresponding output capability. Some are merged behind a helpful name like tool calling or but when you are not familiar with the underlying decisions the name input_multiple is not easily relatable to a vector calculations done by embeddings features. So either this could need a refreshing rename, new capability or some documentation around it on the capability enum.